Add type info to conversion errors. #1050

sebpuetz · 2020-07-18T16:59:23Z

This is a draft to address #652. Initially, I tried to come up with a solution that keeps the downcast error 0-sized by parameterizing PyDowncastError<T1, T2> where T1: PyTypeInfo, T2: PyTypeInfo but I think there are cases where the underlying type is not easily accessible.

I haven't worked much with pyo3 recently, so any pointers if I missed some obvious solution are much appreciated!

Thank you for contributing to pyo3!

Here are some things you should check for submitting your pull request:

Run cargo fmt (This is checked by travis ci)
Run cargo clippy and check there are no hard errors (There are a bunch of existing warnings; This is also checked by travis)
If applicable, add an entry in the changelog.
If applicable, add documentation to all new items and extend the guide.
If applicable, add tests for all new or fixed functions
If you changed any python code, run black .. You can install black with pip install black)

You might want to run tox (pip install tox) locally to check compatibility with all supported python versions. If you're using linux or mac you might find the Makefile helpful for testing.

davidhewitt

🚀 thank you so much for kicking this effort off! I'm very pleased to see this - it's been on my mind for some time and agree it's a critical UX improvement we can make to PyO3!

I have added a few pieces of feedback below.

Also, if you really want to supercharge this, I have two further things I'd love to see:

there's a second trait FromPyObject which has implementations for types like Option<T>. It would be really cool if extraction for that Option<T> mentioned Optional[int] etc as the target type.
It would be 👌 if you added impl std::error::Error for PyDowncastError to this PR!

src/err.rs

src/types/iterator.rs

sebpuetz · 2020-07-18T22:13:30Z

Thanks for the review! I'll amend the PR, probably some time tomorrow!

davidhewitt · 2020-07-18T22:19:50Z

👍 I'll try to remember to merge #1051 in the morning so that you can use that to simplify the PyIterator case. Feel free to ping me if I forget!

sebpuetz · 2020-07-18T23:41:43Z

there's a second trait FromPyObject which has implementations for types like Option<T>. It would be really cool if extraction for that Option<T> mentioned Optional[int] etc as the target type.

I spent some time trying to implement this, but I don't think it's easily doable:

impl<'a, T> FromPyObject<'a> for Option<T>
where
    T: FromPyObject<'a>,
{
    fn extract(obj: &'a PyAny) -> PyResult<Self> {
        if obj.as_ptr() == unsafe { ffi::Py_None() } {
            Ok(None)
        } else {
            match T::extract(obj) {
                Ok(v) => Ok(Some(v)),
                Err(e) => Err(e),
            }
        }
    }
}

assumes that T is any Rust type that implements FromPyObject, so we don't get any PyTypeInfo about T. T::extract() implementations know the target types of the casts, but these details aren't available on T.

Unless I'm missing something, PyErr only makes the message available through its pvalue, which is already some Python type that in itself requires extracting a String. We'd have to ensure that the error is in fact a TypeError and that it's a message that we generated in order to manipulate the to part. This seems quite messy and error prone.

In other words, I don't think we can get the nice Option messages without changing extract errors to be generic over E: Into<PyErr>...

edit: Anyways, I might open a PR tomorrow to simplify the match, T::extract(obj).map(|v| Some(v)) should be more concise!

davidhewitt · 2020-07-19T07:47:39Z

Ah right, thanks for exploring the Option case further. I agree let's not pursue that right now - maybe I'll rework another time though as you say will be more breaking. The mini PR would be very welcome though!

I've merged #1051 so you should be unblocked here now!

kngwyu

Thank you, but I don't think the lifetime 'b matters.

src/err.rs

davidhewitt

Thanks, this is now looking excellent to me! I have one final suggestion to handle the case when repr() fails.

If I can make one further ask, could you please add a CHANGELOG entry? I suggest:

- Add additional context to `PyDowncastError`. [#1050](https://github.com/PyO3/pyo3/pull/1050)

davidhewitt · 2020-07-19T09:58:54Z

src/err.rs

+        write!(
+            f,
+            "Can't convert {} to {}",
+            self.from.repr().map_err(|_| std::fmt::Error)?,


I think in the error case we can fall back to showing the type as you did before:

Suggested change

self.from.repr().map_err(|_| std::fmt::Error)?,

self.from.repr().unwrap_or_else(|_| format!("object of type `{}`", self.from.get_type().name())),

sebpuetz · 2020-07-19T10:09:07Z

If I can make one further ask, could you please add a CHANGELOG entry? I suggest:

I had already amended the previous commit with:

- Implement type information for conversion failures  [#1050](https://github.com/PyO3/pyo3/pull/1050)

I pushed out the version with the fallback to the from type!

davidhewitt · 2020-07-19T10:17:17Z

Ah brilliant, thanks very much, I missed that. Once CI passes let's get this merged! 🚀

davidhewitt

Oh, actually one last little thing: looks like there's still some 'b lifetimes unused which we can remove...

src/conversion.rs

davidhewitt

I went ahead and applied that change. Sorry if that breaks it! 😄

davidhewitt · 2020-07-19T11:01:21Z

CI is happy so let's merge it! Many thanks again

gilescope · 2020-10-06T16:47:35Z

src/err.rs

@@ -56,7 +57,20 @@ pub struct PyErr {
 pub type PyResult<T> = Result<T, PyErr>;

 /// Marker type that indicates an error while downcasting


technically we should stop calling it a marker type now.

davidhewitt reviewed Jul 18, 2020

View reviewed changes

src/err.rs Outdated Show resolved Hide resolved

src/err.rs Outdated Show resolved Hide resolved

src/err.rs Outdated Show resolved Hide resolved

src/types/iterator.rs Outdated Show resolved Hide resolved

davidhewitt mentioned this pull request Jul 18, 2020

Change PyIterator::from_object` to return underlying TypeError #1051

Merged

sebpuetz force-pushed the conversion-errors branch from 6bda2f9 to e8596dd Compare July 18, 2020 23:54

kngwyu requested changes Jul 19, 2020

View reviewed changes

src/err.rs Outdated Show resolved Hide resolved

kngwyu reviewed Jul 19, 2020

View reviewed changes

src/err.rs Outdated Show resolved Hide resolved

kngwyu reviewed Jul 19, 2020

View reviewed changes

src/err.rs Outdated Show resolved Hide resolved

sebpuetz force-pushed the conversion-errors branch from e8596dd to 0817b54 Compare July 19, 2020 08:53

sebpuetz marked this pull request as ready for review July 19, 2020 08:54

davidhewitt reviewed Jul 19, 2020

View reviewed changes

Add type info to conversion errors.

63d6d4c

sebpuetz force-pushed the conversion-errors branch from 0817b54 to 63d6d4c Compare July 19, 2020 10:08

davidhewitt requested changes Jul 19, 2020

View reviewed changes

Remove redundant lifetimes

a058eb5

davidhewitt approved these changes Jul 19, 2020

View reviewed changes

davidhewitt merged commit 53a248c into PyO3:master Jul 19, 2020

davidhewitt mentioned this pull request Jul 19, 2020

More verbose conversion/downcasting errors. #652

Closed

sebpuetz mentioned this pull request Jul 19, 2020

Remove explicit matches in conversions. #1053

Merged

davidhewitt mentioned this pull request Jul 20, 2020

Function argument error handling improvements #1055

Open

gilescope reviewed Oct 6, 2020

View reviewed changes

davidhewitt mentioned this pull request Oct 10, 2020

Tweak doc for PyDowncastError #1223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type info to conversion errors. #1050

Add type info to conversion errors. #1050

sebpuetz commented Jul 18, 2020 •

edited

Loading

davidhewitt left a comment

sebpuetz commented Jul 18, 2020

davidhewitt commented Jul 18, 2020

sebpuetz commented Jul 18, 2020 •

edited

Loading

davidhewitt commented Jul 19, 2020

kngwyu left a comment

davidhewitt left a comment

davidhewitt Jul 19, 2020

sebpuetz commented Jul 19, 2020

davidhewitt commented Jul 19, 2020

davidhewitt left a comment

davidhewitt left a comment

davidhewitt commented Jul 19, 2020

gilescope Oct 6, 2020

	self.from.repr().map_err(\|_\| std::fmt::Error)?,
	self.from.repr().unwrap_or_else(\|_\| format!("object of type `{}`", self.from.get_type().name())),

		@@ -56,7 +57,20 @@ pub struct PyErr {
		pub type PyResult<T> = Result<T, PyErr>;

		/// Marker type that indicates an error while downcasting

Add type info to conversion errors. #1050

Add type info to conversion errors. #1050

Conversation

sebpuetz commented Jul 18, 2020 • edited Loading

davidhewitt left a comment

Choose a reason for hiding this comment

sebpuetz commented Jul 18, 2020

davidhewitt commented Jul 18, 2020

sebpuetz commented Jul 18, 2020 • edited Loading

davidhewitt commented Jul 19, 2020

kngwyu left a comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt Jul 19, 2020

Choose a reason for hiding this comment

sebpuetz commented Jul 19, 2020

davidhewitt commented Jul 19, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt commented Jul 19, 2020

gilescope Oct 6, 2020

Choose a reason for hiding this comment

sebpuetz commented Jul 18, 2020 •

edited

Loading

sebpuetz commented Jul 18, 2020 •

edited

Loading